626525 - Disable "move to group" and "next group" if Panorama hasn't been run

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Ian Gilman [:iangilman]]

As it is right now, right clicking on a tab in the tab bar spins up Panorama in the background, in order to support the "move to group" item on the context menu. Hitting the "next group" key combo does the same. 

In the spirit of bug 626500, attempting to make sure people's first Panorama experience is intentional (i.e. selected explicitly from the menu bar), I propose that we disable both of the above features until Panorama has been run once (as determined by the browser.panorama.experienced_firstrun pref).

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we make it so Panorama is only spun up when the Move to Group context item is hovered long enough for the submenu to pop open?

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: WIP

This patch disables the "next group" key combination until Panorama has been run
once. 

Also, it makes Panorama is only spun up when the "Move to Group" menu
is hovered more than 2 secs for the submenu to pop open if Panorama hasn't been started before in the same browsing session.  Or we should just keep this behaviour only until Panorama has been run once in the app lifetime?

Comment 3

[Dão Gottwald [:dao]]

Why would we load the iframe at all when there aren't any hidden tabs?

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> Why would we load the iframe at all when there aren't any hidden tabs?

Actually we don't need to load when there aren't any hidden tabs.  We can do these
* If there aren't any hidden tabs, we only load the iframe if user clicks on "Move To Group" > "New Group"
* If there are more than one hidden tab and the "Move to Group" menu
is hovered more than 2 secs, the iframe would then be loaded and the submenu is shown.

WYT?

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> * If there are more than one hidden tab and the "Move to Group" menu
> is hovered more than 2 secs, the iframe would then be loaded and the submenu is
> shown.

We don't need a delay in that case.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 504657 [details] [diff] [review]
WIP

>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          Services.prefs.getBoolPref("browser.panorama.experienced_first_run"))
>         return;

Wait, I'm confused. Doesn't this mean we ignore the keypress if Panorama is visible *or* if we're a Panorama user?

Don't you want to check for !Services.prefs.getBoolPref... ?

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Comment on attachment 504657 [details] [diff] [review]
> WIP
> 
> >     window.addEventListener("keypress", function(event) {
> >-      if (self.isVisible())
> >+      if (self.isVisible() ||
> >+          Services.prefs.getBoolPref("browser.panorama.experienced_first_run"))
> >         return;
> 
> Wait, I'm confused. Doesn't this mean we ignore the keypress if Panorama is
> visible *or* if we're a Panorama user?
> 
> Don't you want to check for !Services.prefs.getBoolPref... ?

Yes, we should check for !Services.prefs.getBoolPref...  This patch is WIP so I haven't tested that yet.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

I think we may want to disable this entirely until panorama has a primary UI control by default.  There isn't necessarily a good correlation between if a Firefox profile has triggered panorama once, and if the user is aware of the feature (old computers, shared computers, etc.)  Also, when we turn this back on I think it should only be for named groups, so we can make a cleaner distinction between bookmarks and the user's session (more discussion about that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to comment #8)
> I think we may want to disable this entirely until panorama has a primary UI
> control by default.  There isn't necessarily a good correlation between if a
> Firefox profile has triggered panorama once, and if the user is aware of the
> feature (old computers, shared computers, etc.)  Also, when we turn this back
> on I think it should only be for named groups, so we can make a cleaner
> distinction between bookmarks and the user's session (more discussion about
> that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )

For now, do you mean we should disable the following items until user places the Panorama button to the tabstrip manually?
* tab context menu - "Move To Tab"
* key combination to next group
* View menu > Tab Groups

Comment 10

[Kevin Hanes]

(In reply to comment #8)
> I think we may want to disable this entirely until panorama has a primary UI
> control by default.  There isn't necessarily a good correlation between if a
> Firefox profile has triggered panorama once, and if the user is aware of the
> feature (old computers, shared computers, etc.)  Also, when we turn this back
> on I think it should only be for named groups, so we can make a cleaner
> distinction between bookmarks and the user's session (more discussion about
> that here: https://bugzilla.mozilla.org/show_bug.cgi?id=622452#c8 )

Do you mean that this menu item should not allow a user to move a tab to a new group?

I agree that move-to-group shouldn't list anything but named groups, but I think it's important that the user be able to move a tab to a new group from the tabbed browser.

Comment 11

[Ian Gilman [:iangilman]]

I propose this spec:

* "Move to group" is disabled entirely if Panorama's "first run" flag is off. Otherwise it doesn't spin up Panorama unless there are hidden tabs or you select "new group".

* "Next group" is disabled if there are not hidden tabs.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: WIP 2

Will add test shortly

Comment 13

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 14

[Kevin Hanes]

bugspam. Removing b10

Comment 16

[Frank Yan (:fryn)]

Adding keywords to reflect the following user experience:

1. Open two tabs.
2. Right-click the second tab, and select "Move to" -> "New Group".
Only the first tab is left, with no indication of where the other went.
To a user, who has never used Panorama, this would be perceived as dataloss.
Meanwhile, the tab is still there, so Firefox would continue to use just as much memory.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 18

[Raymond Lee [:raymondlee]]

(In reply to comment #16)
> Adding keywords to reflect the following user experience:
> 
> 1. Open two tabs.
> 2. Right-click the second tab, and select "Move to" -> "New Group".
> Only the first tab is left, with no indication of where the other went.
> To a user, who has never used Panorama, this would be perceived as dataloss.
> Meanwhile, the tab is still there, so Firefox would continue to use just as
> much memory.

Hi Frank, could you file another for this please? This bug is about disabling the "move to group" and "next group".

Comment 19

[Frank Yan (:fryn)]

(In reply to comment #18)
Filed: bug 628235

Comment 20

[Frank Yan (:fryn)]

Comment on attachment 506328 [details] [diff] [review]
v1

>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

It looks like this will prevent use of the tab view shortcut when all tabs are in one group, even if a user wants to use the spatial interface to separate tabs into multiple groups.

Also, why isn't panorama using a <key/> in browser.xul for its keyboard shortcut?
AIUI, it would be superior in terms of performance.
Attaching event handlers to |window| via JS generally is avoided in browser window code.

Comment 21

[Raymond Lee [:raymondlee]]

(In reply to comment #20)
> Comment on attachment 506328 [details] [diff] [review]
> v1
> 
> >     window.addEventListener("keypress", function(event) {
> >-      if (self.isVisible())
> >+      if (self.isVisible() ||
> >+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
> >         return;
> 
> It looks like this will prevent use of the tab view shortcut when all tabs are
> in one group, even if a user wants to use the spatial interface to separate
> tabs into multiple groups.
> 

Please have a look at this.  The keypress is for next/previous group key combination.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabview.js#224

> Also, why isn't panorama using a <key/> in browser.xul for its keyboard
> shortcut?
> AIUI, it would be superior in terms of performance.
> Attaching event handlers to |window| via JS generally is avoided in browser
> window code.

I remember that  Control (+ Shift) + ` doesn't work if it's set in a <key/>.  I might be wrong, please find a new bug if you want.

Comment 22

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 506328 [details] [diff] [review]
v1

>     let data = this._sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
...

Move the data = this._sessionstore... down here, before this line:

>     if (data && data == "true") {

>+  // ----------
>+  // On move to group pop showing.
>+  moveToGroupPopupShowing: function Tabview_moveToGroupPopupShowing(event) {
>+    // there are hidden tabs so initialize the iframe and update the context menu
>+    if (gBrowser.tabs.length - gBrowser.visibleTabs.length > 0) {
>+      this.updateContextMenu(TabContextMenu.contextTab, event.target);

Another reason to show the menu would be if there are no hidden tabs, but there are other (empty) groups. Obviously, we can't currently get this information before we init the frame, but we should file a followup for the future (post-bug 588217).

Looks great!

Comment 23

[Alex Faaborg [:faaborg] (Firefox UX)]

If we disable this command (grayed out and inactive), then I think it is important that it has the panorama icon next to it, so that users have a way of figuring out what it is, and how they might go about making it functional.

Otherwise, I think we should opt to remove the item entirely, and have it appear once panorama is active.

Comment 24

[Raymond Lee [:raymondlee]]

Attachment: v2

#comment 22: Updated based on the comment and filed bug 628594
#comment 23: Added the panorama icon next to the "Move to group" menu

Comment 25

[Ian Gilman [:iangilman]]

Comment on attachment 506684 [details] [diff] [review]
v2

Looks good to me, but the browser stuff is outside my domain, so sending to Dao for review.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 506684 [details] [diff] [review]
v2

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        document.getElementById("context_tabViewMenu").disabled = false;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+
>+      document.getElementById("context_tabViewMenu").disabled = true;

Should this be hidden rather than disabled? An item that's universally disabled until some magic steps are executed doesn't seem very useful.

>   // ----------
>   // Adds new key commands to the browser, for invoking the Tab Candy UI
>   // and for switching between groups of tabs when outside of the Tab Candy UI.
>-  _setBrowserKeyHandlers : function() {
>+  _setBrowserKeyHandlers: function() {
>     let self = this;
> 
>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

This code shouldn't really run in a window without groups... I don't understand why it still exists at all, the key combo seems quite undiscoverable.

Comment 27

[Ian Gilman [:iangilman]]

(In reply to comment #26)
> Should this be hidden rather than disabled? An item that's universally disabled
> until some magic steps are executed doesn't seem very useful.

In comment 23, Faaborg suggested grayed with an icon, so we went with that.

> This code shouldn't really run in a window without groups... I don't understand
> why it still exists at all, the key combo seems quite undiscoverable.

We definitely want the key combo; it's quite useful for people who use Panorama. I agree that making it more discoverable would be lovely; perhaps we could add a menu item for it?

As for running in a window without groups, we need some way to detect if the window has groups. I suppose we could install the key handler at start up if there are hidden tabs, and if there weren't tabs hidden to begin with, listen for hide events. Does that sound better?

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> (In reply to comment #26)
> > Should this be hidden rather than disabled? An item that's universally disabled
> > until some magic steps are executed doesn't seem very useful.
> 
> In comment 23, Faaborg suggested grayed with an icon, so we went with that.

Ugh, I hadn't noticed the icon part yet, because I just looked at the patch without actually applying it... Context menus aren't really supposed to have icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

> > This code shouldn't really run in a window without groups... I don't understand
> > why it still exists at all, the key combo seems quite undiscoverable.
> 
> We definitely want the key combo; it's quite useful for people who use
> Panorama. I agree that making it more discoverable would be lovely; perhaps we
> could add a menu item for it?

We could, but not for Firefox 4, since we don't have strings for it afaik. I don't think there's a point in keeping the key combo, especially if it adds overhead to key events processing for all users, as long as it's not discoverable. People just aren't going to know about it.

> As for running in a window without groups, we need some way to detect if the
> window has groups. I suppose we could install the key handler at start up if
> there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> for hide events. Does that sound better?

That's not sufficient, since tabs can be hidden outside of panorama land. See e.g. http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343

Comment 29

[Ian Gilman [:iangilman]]

(In reply to comment #28)
> Ugh, I hadn't noticed the icon part yet, because I just looked at the patch
> without actually applying it... Context menus aren't really supposed to have
> icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

Fair enough; that's fine with me.

> We could, but not for Firefox 4, since we don't have strings for it afaik. I
> don't think there's a point in keeping the key combo, especially if it adds
> overhead to key events processing for all users, as long as it's not
> discoverable. People just aren't going to know about it.

It's a shame it's not discoverable, but it's an important key-combo... it's a very popular request. We'll just have to settle for spreading the word via Help and word of mouth.

> > As for running in a window without groups, we need some way to detect if the
> > window has groups. I suppose we could install the key handler at start up if
> > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > for hide events. Does that sound better?
> 
> That's not sufficient, since tabs can be hidden outside of panorama land. See
> e.g.
> http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343

I was referring to the hide event: 

http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947

That should cover it, right?

Comment 30

[Alex Faaborg [:faaborg] (Firefox UX)]

>Context menus aren't really supposed to have
>icons. Hiding the item (also mentioned in comment 23) seems much cleaner.

We're going to give significant context menu items icons in the next release, so we might as well start now.  Icons in context menus is native on Vista and 7 (but only for some items in each menu).  We need to provide the user with some form of clue as to how they could get this command enabled, and the icon does that.

>I don't understand why it still exists at all, the key combo seems quite
>undiscoverable.

I agree that the key combo is undiscoverable in the product itself, but it's a useful accelerator for advanced users, and there is little risk of users hitting it by accident.  It's a bit of a stretch (literally) and it only works if you've created multiple groups.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> >Context menus aren't really supposed to have
> >icons. Hiding the item (also mentioned in comment 23) seems much cleaner.
> 
> We're going to give significant context menu items icons in the next release,
> so we might as well start now.  Icons in context menus is native on Vista and 7
> (but only for some items in each menu).  We need to provide the user with some
> form of clue as to how they could get this command enabled, and the icon does
> that.

The icon actually does not give a clue, as the panorama button isn't on the toolbar by default.

(In reply to comment #29)
> > > As for running in a window without groups, we need some way to detect if the
> > > window has groups. I suppose we could install the key handler at start up if
> > > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > > for hide events. Does that sound better?
> > 
> > That's not sufficient, since tabs can be hidden outside of panorama land. See
> > e.g.
> > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343
> 
> I was referring to the hide event: 
> 
> http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947
> 
> That should cover it, right?

The code I linked to triggers the event.

Comment 32

[Ian Gilman [:iangilman]]

(In reply to comment #31)
> > We're going to give significant context menu items icons in the next release,
> > so we might as well start now.  Icons in context menus is native on Vista and 7
> > (but only for some items in each menu).  We need to provide the user with some
> > form of clue as to how they could get this command enabled, and the icon does
> > that.
> 
> The icon actually does not give a clue, as the panorama button isn't on the
> toolbar by default.

That icon is also being used for the Panorama menu item (that's being moved to the "list all tabs" menu), see bug 625320; we need it on both of them to tie it together.

> > > > As for running in a window without groups, we need some way to detect if the
> > > > window has groups. I suppose we could install the key handler at start up if
> > > > there are hidden tabs, and if there weren't tabs hidden to begin with, listen
> > > > for hide events. Does that sound better?
> > > 
> > > That's not sufficient, since tabs can be hidden outside of panorama land. See
> > > e.g.
> > > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/browser.js#l8343
> > 
> > I was referring to the hide event: 
> > 
> > http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/browser/base/content/tabbrowser.xml#l1947
> > 
> > That should cover it, right?
> 
> The code I linked to triggers the event.

Ah, you mean it'll be a false positive? Sorry, I didn't catch that. Good point. 

I think tabs being in a hidden state on startup is a pretty good indication, though. Let's check for that at startup, and attach the handler then if there are hidden tabs. Otherwise we should attach the handler when Panorama is first started during the session.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> (In reply to comment #31)
> > > We're going to give significant context menu items icons in the next release,
> > > so we might as well start now.  Icons in context menus is native on Vista and 7
> > > (but only for some items in each menu).  We need to provide the user with some
> > > form of clue as to how they could get this command enabled, and the icon does
> > > that.
> > 
> > The icon actually does not give a clue, as the panorama button isn't on the
> > toolbar by default.
> 
> That icon is also being used for the Panorama menu item (that's being moved to
> the "list all tabs" menu), see bug 625320; we need it on both of them to tie it
> together.

Can we please fix this bug in the most straightforward way and move on? An always-disabled item just isn't helpful, bug 625320 doesn't change this.

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

>The icon actually does not give a clue, as the panorama button isn't on the
>toolbar by default.

>An always-disabled item just isn't helpful, bug 625320 doesn't change this.

Here's the sequence of actions I'm hoping for (although not necessarily in this order)

1) the user encounters "move to group" with a unique symbol (4 interlocking squares) and thinks "hm, seems interesting but isn't enabled, I wonder what the deal with that is"

2) user clicks on list all tabs, and perhaps they have a tremendously large number of tabs open (which is entirely possible if they are taking advantage of the list all tabs menu).  At the top is the solution to the problem that they didn't even know they had: "Group your tabs."  again the user sees a unique symbol of 4 interlocking squares

[2.5 - optional - the user reads a review of Firefox 4 that covers panorama and tab groups, and shows them the unique symbol for the feature, introducing what it does]

3) now a toolbar button is available with that same unique symbol, and (if they have created another group) the context menu action is turned on.  Even if the user forgets about one of the entry points (or a new user sits down at the machine) the three interfaces visually reference each other.

Comment 35

[Dão Gottwald [:dao]]

2) and 3) make sense, I just don't see the critical role of 1) in there. 2) might happen weeks after 1), or never. Why would we intentionally puzzle the user like this?

Comment 36

[Raymond Lee [:raymondlee]]

Attachment: v3

Hide the "Move to group" menu item if the experienced_first_run pref hasn't been set.  Keeping the icon for the "Move to group" menu item.

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 508324 [details] [diff] [review]
v3

r- on the browser/themes/ part, menu icons need to be 16*16px.

Comment 38

[Raymond Lee [:raymondlee]]

Attachment: v4

Checked and updated the CSS for 16x16 icons.  They look fine on different platforms

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 508331 [details] [diff] [review]
v4

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css
>@@ -582,16 +582,21 @@ toolbar[iconsize="small"][mode="icons"] 
> #tabview-button[groups="2"] {
>   -moz-image-region: rect(0, 60px, 20px, 40px);
> }
> 
> #tabview-button[groups="3"] {
>   -moz-image-region: rect(0, 80px, 20px, 60px);
> }
> 
>+#context_tabViewMenu {
>+  list-style-image: url(chrome://browser/skin/tabview/tabview.png);
>+  -moz-image-region: rect(2px, 18px, 18px, 2px);
>+}

I think this might be a problem on OS X (cf. bug 603605), but I have no way of testing this.
Can you move the iconification to a separate bug? I don't think it belongs in here.

Comment 40

[Raymond Lee [:raymondlee]]

Attachment: v5

Removed the icons and filed bug 630128

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 508334 [details] [diff] [review]
v5

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;
>+
>+    // ___ keys
>+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
>+      this._setBrowserKeyHandlers();

Will session restore already have hidden the tabs at this point?

>     // ___ visibility
>     this._sessionstore =
>       Cc["@mozilla.org/browser/sessionstore;1"].
>         getService(Ci.nsISessionStore);

Btw, why this._sessionstore rather than a local variable?

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+      this._firstRunExperienced = false;

nit: last line is redundant

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+    document.getElementById("context_tabViewMenu").hidden = 

nit: trailing space

Comment 42

[Raymond Lee [:raymondlee]]

Attachment: v5

(In reply to comment #41)
> Comment on attachment 508334 [details] [diff] [review]
> v5
> 
> >   init: function TabView_init() {
> >-    // ___ keys    
> >-    this._setBrowserKeyHandlers();
> >+    let self = this;
> >+
> >+    // ___ keys
> >+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> >+      this._setBrowserKeyHandlers();
> 
> Will session restore already have hidden the tabs at this point?

Yes, I've checked that the session restore already loads and hides the tabs before this point.

> 
> >     // ___ visibility
> >     this._sessionstore =
> >       Cc["@mozilla.org/browser/sessionstore;1"].
> >         getService(Ci.nsISessionStore);
> 
> Btw, why this._sessionstore rather than a local variable?

Fixed

> 
> >+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
> >+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
> >+      let prefsObserver = function() {
> >+        Services.prefs.removeObserver(
> >+          "browser.panorama.experienced_first_run", prefsObserver);
> >+        self._firstRunExperienced = true;
> >+      };
> >+      Services.prefs.addObserver(
> >+        "browser.panorama.experienced_first_run", prefsObserver, false);
> >+      this._firstRunExperienced = false;
> 
> nit: last line is redundant

Removed.

> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> >+    document.getElementById("context_tabViewMenu").hidden = 
> 
> nit: trailing space

Fixed.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 508338 [details] [diff] [review]
v5

>--- a/browser/base/content/browser-tabview.js
>+++ b/browser/base/content/browser-tabview.js

>   _sessionstore: null,

This is now unused.

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;
>+
>+    // ___ keys
>+    if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
>+      this._setBrowserKeyHandlers();
>+
>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);
>+    } else {
>+      this._firstRunExperienced = true;
>+    }
> 
>     // ___ visibility
>-    this._sessionstore =
>-      Cc["@mozilla.org/browser/sessionstore;1"].
>-        getService(Ci.nsISessionStore);
>-
>-    let data = this._sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
>+    let sessionstore =
>+      Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>+    let data = sessionstore.getWindowValue(window, this.VISIBILITY_IDENTIFIER);
> 
>     if (data && data == "true") {
>       this.show();
>     } else {

Can you avoid some work for browser.panorama.experienced_first_run = false, like checking ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) and getting the sessionstore service?

Comment 44

[Raymond Lee [:raymondlee]]

Attachment: v6

Removed some work for browser.panorama.experienced_first_run = false i.e.
checking ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) and
getting the sessionstore service.

Comment 45

[Dão Gottwald [:dao]]

Comment on attachment 508670 [details] [diff] [review]
v6

>   init: function TabView_init() {
>-    // ___ keys    
>-    this._setBrowserKeyHandlers();
>+    let self = this;

Please move this down to the branches it's needed in.

>+  _setBrowserKeyHandlers: function() {
>     let self = this;
> 
>+    if (this._browserKeyHandlerInitialized)
>+      return;
>+
>+    this._browserKeyHandlerInitialized = true;
>+
>     window.addEventListener("keypress", function(event) {
>-      if (self.isVisible())
>+      if (self.isVisible() ||
>+          (gBrowser.tabs.length - gBrowser.visibleTabs.length) == 0)
>         return;

let self = this; should be right before window.addEventListener.

The test is using 'win.window', which I suspect is going to be identical to 'win', in which case you should write just that.

Comment 46

[Dão Gottwald [:dao]]

Comment on attachment 508670 [details] [diff] [review]
v6

>+    if (!Services.prefs.prefHasUserValue("browser.panorama.experienced_first_run") ||
>+        !Services.prefs.getBoolPref("browser.panorama.experienced_first_run")) {
>+      let prefsObserver = function() {
>+        Services.prefs.removeObserver(
>+          "browser.panorama.experienced_first_run", prefsObserver);
>+        self._firstRunExperienced = true;
>+      };
>+      Services.prefs.addObserver(
>+        "browser.panorama.experienced_first_run", prefsObserver, false);

Actually, I think you need to remove the observer when the window closes. To that end, can you make the TabView object implement an 'observe' method and directly add/remove 'this' as the observer?

Comment 47

[Alex Faaborg [:faaborg] (Firefox UX)]

>2) and 3) make sense, I just don't see the critical role of 1)

Critical role is probably overselling it, but I do think that users who currently rely on the command "bookmark all tabs" in that menu are going to become curious about the idea of tab groups, since it is pretty much built exactly for them.

Also, the existence of the icon isn't meant to make it super special (as in the only context menu item in all of Firefox that has an icon), but rather just special (one of the top 2 or 3 items in that particular menu that has an icon.

Comment 48

[Alex Faaborg [:faaborg] (Firefox UX)]

Related comment on this not being the only icon in the menu: https://bugzilla.mozilla.org/show_bug.cgi?id=630128#c15

Comment 49

[Alex Faaborg [:faaborg] (Firefox UX)]

>uiwanted
ignore, just testing out search > whine event > email > sms

Comment 50

[Raymond Lee [:raymondlee]]

Attachment: v7

Comment 51

[Dão Gottwald [:dao]]

(In reply to comment #47)
> >2) and 3) make sense, I just don't see the critical role of 1)
> 
> Critical role is probably overselling it, but I do think that users who
> currently rely on the command "bookmark all tabs" in that menu are going to
> become curious about the idea of tab groups, since it is pretty much built
> exactly for them.

Unfortunately we can't detect these users. (Well, technically we could, but we don't do it right now, and the idea seems a bit strange.) I've never seen anyone bookmark all tabs. If it's as low-usage as I suspect, it shouldn't really be considered relevant here.

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 509056 [details] [diff] [review]
v7

>+  uninit: function TabView_uninit() {

>+    if (this._tabShowEventListener) {
>+      gBrowser.tabContainer.removeEventListener(
>         "TabShow", this._tabShowEventListener, true);
>     }

This should be needed, as this event listener shouldn't keep the window alive.

Comment 53

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Passed Try http://tbpl.mozilla.org/?tree=MozillaTry&rev=32cc4544cc9d

Comment 54

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f2a6a78478dc

Comment 55

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 56

[Lars Gunther]

In my nightly "move to group" is disabled even if I have run Panorama. Which I use a lot, BTW, and this really annoys me, considering the fact that the feature has become part of my regular usage pattern.

I does not appear to be bug 628594 because I do have other named groups. (I have 7 groups, all named.)

Should I file a bug?

Comment 57

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #56)
> In my nightly "move to group" is disabled even if I have run Panorama. Which I
> use a lot, BTW, and this really annoys me, considering the fact that the
> feature has become part of my regular usage pattern.

This will be fixed by bug 635051, sorry for the inconvenience.

> I does not appear to be bug 628594 because I do have other named groups. (I
> have 7 groups, all named.)

For a temporary workaround you can just create a new group and close it. After that the menu items should be available again.

Comment 58

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm on the 2/22 nightly, am a regular panorama users, and don't see this command at all (completely gone, as opposed to being present and disabled).  Is there another bug still open, or should re re-open this one.

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #58)
> I'm on the 2/22 nightly, am a regular panorama users, and don't see this
> command at all (completely gone, as opposed to being present and disabled).  Is
> there another bug still open, or should re re-open this one.

It's bug 635051 that is fixed now but unfortunately not in b12. You can set the name of some group in panorama and the menu items are permanently enabled again.